This fails to validate the known hooks if something other than an array is provided, which will result in an error upstream.

Nope. https://github.com/szmarczak/got/blob/b22c3abbe8daf905aae4bed66cddfc7b9ed72170/source/normalize-arguments.js#L191-L199

Ah, thanks - I got tripped up reviewing the diff.

This is not equivalent to the current implementation. Consider these examples:

const a = [1, 2];
a.foo = 'bar';
Object.entries(a); // [ [ '0', 1 ], [ '1', 2 ], [ 'foo', 1 ] ]

It's not a bug. It's a feature!

The point of this test is to show that extra keys are not required to pass validation b/c they should be ignored. The test title could be more clear.

Why would someone assign non-hook object to hooks? What's the use case? What big problem does this solve? Sorry, but I don't see that.

It allows for encapsulation. Here's a contrived example:

class GotHooks {

  constructor () {
    this.message = 'Encapsulated Got Hooks';
    this.beforeRequest = [
      () => this.runRook()
    ];
  }

  async runRook() {
    console.log(this.message);
  }
}

If got validates only the properties it is concerned with, then everything is fine. If got extraneously demands that the object only have properties that correspond to known hooks, then the user has to jump through some hoops to create an object that accomplishes the same thing.

Thank you for the detailed response, I really appreciate that! 👍

I'm not sure what the origin of this conditional block are, but I think the right way to handle this event is to remove the if block and make this:

req.on('socket', () => { ... });

https://nodejs.org/dist/latest-v8.x/docs/api/http.html#http_event_socket

Actually, since onSocketConnect and onAbort should be attached to the request, I don't think these are sensible hooks for got to configure directly. If you want to configure these for a got instance, you really just need to configure a listener for request and attach the request event listeners in the request listener.

I'm not sure what the origin of this conditional block are, but I think the right way to handle this event is to remove the if block

#429

Actually, since onSocketConnect and onAbort should be attached to the request, I don't think these are sensible hooks for got to configure directly. If you want to configure these for a got instance, you really just need to configure a listener for request and attach the request event listeners in the request listener.

Why would you do the same thing twice? Oh wait. I do that twice. I'll change that.

Thanks for the reference to #429. Maybe I'm missing something, but I don't think that was the right fix b/c if req.connection is just testing a race condition. Shouldn't it be:

req.once('socket', (sock) => {
  sock.once('connect', conn => {
    // ...
  }
};

I can see the convenience of low-friction hooks. Maybe there's a way to make the configuration a sort of DSL that got can use to wire these up. Something like this:

{
  events: {
    request: {
      ['on|once']: {
        socket: {
          null: [(sock) => { console.log('request.on(socket)') }],
          connect: [(conn) => { console.log('socket.on(connect)') }]
        }
      },
    }
  }
}

With the above, the configuration can be traversed and the listeners can be subscribed without having to add handling code to got for every event.

I used null as a sentinel value to mean the subscriber for the event as opposed to a child subscription. Something else (symbol?) would work also.

Maybe I'm missing something, but I don't think that was the right fix b/c if req.connection is just testing a race condition. Shouldn't it be:

I don't know. Can you make another issue for that?

I can see the convenience of low-friction hooks.

Great! We'll discuss this later. I was gonna send a PR with rewrited hooks from scratch but I decided current implementation is better. :)